Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional support for windows #475

Merged
merged 9 commits into from
May 5, 2021
Merged

Additional support for windows #475

merged 9 commits into from
May 5, 2021

Conversation

jenhagg
Copy link
Collaborator

@jenhagg jenhagg commented May 3, 2021

Purpose

Address several places that were not platform independent.

What the code is doing

  • Finished splitting up commands between local/ssh and removed the execute_command in the local case, as it relied on /bin/bash
  • Add the DataAccess.join attribute according to the subclass, removing hard coded posixpath.join in calling code
  • Added get_base_dir and match_scenario_files to encapsulate the logic of creating paths, along with tests for these
  • Fix potential issue in InputData and OutputData which were relying on the values from server_setup.py, which had posixpath implicitly hard coded (e.g. INPUT_DIR = data/input). We delay the path join by using a tuple instead

Testing

  • Ran a scenario in docker (using LocalDataAccess)
  • Created and prepared a scenario on the server and deleted it (using SSHDataAccess)
  • Added a few unit tests

Time estimate

20 mins

@jenhagg jenhagg self-assigned this May 3, 2021
print(f"--> Moving file {src} to {dest}")
self._check_file_exists(dest, should_exist=False)
self.copy(src, dest)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoiding the use of copy because dest is a file, and we don't it to automatically create /path/to/file.txt/ as a folder

@@ -41,25 +40,21 @@ def delete_scenario(self, confirm=True):
self._scenario_list_manager.delete_entry(scenario_id)
self._execute_list_manager.delete_entry(scenario_id)

wildcard = f"{scenario_id}_*"

print("--> Deleting scenario input data on server")
Copy link
Collaborator

@rouille rouille May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we delete locally when using plug. Printing "Deleting scenario input data" will then be always correct. Or perhaps, we can take advantage of the description attribute to custom the message

print("--> Deleting scenario input data on server")
target = posixpath.join(self.path_config.input_dir(), wildcard)
target = self._data_access.match_scenario_files(scenario_id, "input")
self._data_access.remove(target, recursive=False, confirm=confirm)

print("--> Deleting scenario output data on server")
Copy link
Collaborator

@rouille rouille May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here

self._data_access.remove(tmp_dir, recursive=True, confirm=confirm)

print("--> Deleting input and output data on local machine")
target = os.path.join(server_setup.LOCAL_DIR, "data", "**", wildcard)
target = os.path.join(server_setup.LOCAL_DIR, "data", "**", f"{scenario_id}_*")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do a LocalDataAccess().join here and get rid of the import os. It will be nice to be fully OS independent in the class.

)
self.TMP_DIR = self._data_access.match_scenario_files(self.scenario_id, "tmp")

def create_folder(self):
"""Creates folder on server that will enclose simulation inputs."""
print("--> Creating temporary folder on server for simulation inputs")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the the temporary folder can be crated either on the local machine or the server. Perhaps we can the description attribute here too.

raise IOError(f"Failed to copy {kind}.csv on server")
from_dir = self._data_access.match_scenario_files(profile_as, "tmp")
to_dir = self.TMP_DIR
self._data_access.copy(f"{from_dir}/{kind}.csv", to_dir)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is kind of confusing to move to using relative path but to copy with absolute path

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I just didn't know a cleaner way to do it. The reason for this is mainly how we use copy within move.py, where the destination (backup disk) is not relative to the self.root. Maybe we can revisit this in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good

@jenhagg jenhagg force-pushed the jon/paths branch 2 times, most recently from a4f286b to de99742 Compare May 5, 2021 20:46
Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@jenhagg
Copy link
Collaborator Author

jenhagg commented May 5, 2021

Rebased and fixed the test that was still calling data_access._execute_command("whoami")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants